-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: generate notification conversation deleted - WPB-11658 #2381
base: refactor/generate-notification-conversation-member-leave
Are you sure you want to change the base?
Conversation
Test Results 3 files 4 suites 4m 19s ⏱️ Results for commit 8ac66ab. |
Datadog ReportBranch report: ✅ 0 Failed, 3199 Passed, 28 Skipped, 2m 14.72s Total Time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM;)
.deletedGroup(senderName: context.senderName) | ||
) | ||
|
||
content.body = body.make() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: one of the things that I don't like in our legacy code is how the actual values for the notification content separated from the content
object because it usually means that there is some work involved in understanding exactly how a notification is configured.
I know that in some cases there is some complexity in generating content such as the body, and that it should also be reused among various notifications types, but I believe that mostly applies to new message notifications. In this case, I think it would be better to have the actual values closer to the content
, something like:
content.body = "A conversation was deleted"
content.categoryIdentifier = .nonActionable
content.sound = .click
What do you think?
Key points
This PR is about generating notification content related to the conversation deleted event populating the right data in the notification (title, body, sound, category..).
Testing
Checklist
[WPB-XXX]
.